CRaC may exit before image dump is completed #46

wkia · 2023-02-21T13:50:23Z

When running CRaC with docker, java may exit before CRIU is finished dumpring because CRIU kills the original java process, and then docker immediately exits.

It could be reproduced with a simple Java test:

public class Test {
    public static void main(String args[]) throws Exception {
        jdk.crac.Core.checkpointRestore();
        System.out.println("finish");
    }
}

and run with docker:
docker <docker_options> $JAVA_HOME/java -XX:CRaCCheckpointTo=./cr Test.java

After the command above finishes, cr/cppath is absent in the case of failure. Or/and it will fail on restore:
docker <docker_options> $JAVA_HOME/java -XX:CRaCRestoreFrom=./cr

This change fixes the issue by forkin'g the main process in case of PID=1 (pid=1 means it was run with docker), and waiting for children processes are finished. This makes us sure that CRIU finalized the dump, if any. At the same time, there is no conflict with PIDs on restore, since the process being restored has PID not equal to 1, if restoring with the command above..

Progress

Change must not contain extraneous whitespace

Reviewers

Anton Kozlov (@AntonKozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/crac pull/46/head:pull/46
$ git checkout pull/46

Update a local copy of the PR:
$ git checkout pull/46
$ git pull https://git.openjdk.org/crac pull/46/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 46

View PR using the GUI difftool:
$ git pr show -t 46

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/crac/pull/46.diff

bridgekeeper · 2023-02-21T13:51:45Z

👋 Welcome back rmarchenko! A progress list of the required criteria for merging this PR into crac will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-02-21T13:52:57Z

@wkia This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

CRaC may exit before image dump is completed

Reviewed-by: akozlov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the crac branch:

dff32bc: Document CRaCCheckpointTo and CRaCRestoreFrom in java(1) man page
80cab69: Fix failing CRaC tests
0738da8: Close extraneous file descriptors
7dfaf5f: DNS cache maintenance

Please see this link for an up-to-date comparison between the source branch of this pull request and the crac branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@AntonKozlov) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mlbridge · 2023-02-21T13:56:46Z

Webrevs

src/java.base/share/native/launcher/main.c

AntonKozlov

LGTM, thanks!

wkia · 2023-03-14T19:24:08Z

/integrate

openjdk · 2023-03-14T19:26:15Z

@wkia
Your change (at version c117d07) is now ready to be sponsored by a Committer.

AntonKozlov · 2023-03-15T14:13:36Z

/sponsor

openjdk · 2023-03-15T14:15:25Z

Going to push as commit a11b46a.
Since your change was applied there have been 4 commits pushed to the crac branch:

dff32bc: Document CRaCCheckpointTo and CRaCRestoreFrom in java(1) man page
80cab69: Fix failing CRaC tests
0738da8: Close extraneous file descriptors
7dfaf5f: DNS cache maintenance

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-15T14:15:44Z

@AntonKozlov @wkia Pushed as commit a11b46a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Implemented fork'ing for pid=1

190841c

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 21, 2023

Minor change

65a6859

DanHeidinga reviewed Feb 21, 2023

View reviewed changes

src/java.base/share/native/launcher/main.c Outdated Show resolved Hide resolved

Added sighandler to the main process

fd074cf

AntonKozlov requested changes Feb 28, 2023

View reviewed changes

src/java.base/share/native/launcher/main.c Outdated Show resolved Hide resolved

Returning status of the child only

a0a7808

wkia requested a review from AntonKozlov March 1, 2023 16:30

AntonKozlov reviewed Mar 1, 2023

View reviewed changes

src/java.base/share/native/launcher/main.c Outdated Show resolved Hide resolved

Fixing review comments

4191fb7

wkia requested a review from AntonKozlov March 7, 2023 09:37

rvansa mentioned this pull request Mar 7, 2023

Add CRaC-specific tests to GHA #48

Closed

1 task

Minor change

c117d07

rvansa mentioned this pull request Mar 7, 2023

Provide way to detect that image is complete CRaC/docs#3

Open

AntonKozlov approved these changes Mar 7, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 14, 2023

openjdk bot added the integrated Pull request has been integrated label Mar 15, 2023

openjdk bot closed this Mar 15, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 15, 2023

wkia deleted the crac_docker_fix branch March 17, 2023 09:24

wkia mentioned this pull request Mar 20, 2023

Implement CRaC exit codes #52

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRaC may exit before image dump is completed #46

CRaC may exit before image dump is completed #46

wkia commented Feb 21, 2023 •

edited by openjdk bot

bridgekeeper bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023 •

edited

mlbridge bot commented Feb 21, 2023 •

edited

AntonKozlov left a comment

wkia commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

AntonKozlov commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

CRaC may exit before image dump is completed #46

CRaC may exit before image dump is completed #46

Conversation

wkia commented Feb 21, 2023 • edited by openjdk bot

Progress

Reviewers

Reviewing

bridgekeeper bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023 • edited

mlbridge bot commented Feb 21, 2023 • edited

Webrevs

AntonKozlov left a comment

Choose a reason for hiding this comment

wkia commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

AntonKozlov commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

wkia commented Feb 21, 2023 •

edited by openjdk bot

openjdk bot commented Feb 21, 2023 •

edited

mlbridge bot commented Feb 21, 2023 •

edited